Open
Conversation
Add product requirements document for uploading structured CI summary data (resource counts, terraform outputs, warnings, errors, output log) to Atmos Pro via the existing instance status PATCH endpoint. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
goruha
added
the
no-release
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Contributor
📝 WalkthroughWalkthroughAdds a new PRD Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Plugin as Component CI Plugin
participant Server as Atmos Pro Server
CLI->>CLI: invoked with --upload-status and shouldUploadStatus == true
CLI->>Plugin: resolve component CI plugin
alt Plugin implements StatusDataProvider
Plugin->>CLI: returns StatusData (map[string]any)
CLI->>CLI: mask sensitive values, base64-encode output_log, set truncated if needed
CLI->>Server: PATCH /instance/status with DTO including `ci` map
else Plugin missing or data build fails
Plugin-->>CLI: warn-only error
CLI->>Server: PATCH /instance/status without `ci` (or partial)
end
Server->>Server: accept/validate `ci` map per PRD contract
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labelsminor Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 59-63: Update the PRD to require explicit secret-redaction and
payload-size limits for the CI output fields: enforce that ci.outputs and the
OutputLog (json:"output_log", referenced as OutputLog) must have sensitive
values masked or dropped (e.g., remove tokens, credentials, secrets), include a
description of the redaction strategy (masking pattern or drop-on-detect) and
add a max payload size limit with behavior on overflow (truncate output, include
truncation metadata and byte counts). Also apply the same requirements to the
other CI output sections noted (the other ci.outputs/ci.output_log occurrences)
so all mentions include the redaction rule, truncation policy, and metadata
requirements.
- Around line 133-140: The PRD shows an unsafe direct type assertion on
result.Data (e.g., result.Data.(*TerraformOutputData).Outputs) which can panic;
update the steps that map ParseOutput()'s OutputResult into InstanceStatusCI to
use a safe assertion pattern: call terraform.ParseOutput(), then check the
assertion like data, ok := result.Data.(*TerraformOutputData) and only read
data.ResourceCounts, data.Outputs (converting TerraformOutput.Value to raw
values), and data.Warnings when ok is true, otherwise set Defaults/empty values
and propagate result.Errors into InstanceStatusCI.Errors; mention the use of the
ok boolean and fallback behavior when mapping HasChanges, HasErrors,
ResourceCounts, Outputs, Warnings, and Errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec11f5ac-c3ff-484f-a00c-90d9c759185f
📒 Files selected for processing (1)
docs/prd/pro-summary-upload.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2259 +/- ##
==========================================
+ Coverage 77.21% 77.27% +0.05%
==========================================
Files 1018 1020 +2
Lines 96368 96481 +113
==========================================
+ Hits 74415 74559 +144
+ Misses 17755 17724 -31
Partials 4198 4198
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…abstraction - Add business problem section explaining the user pain and value proposition - Output log must pass through IO masking layer (Gitleaks patterns) before base64 encoding - Introduce StatusDataProvider interface for component-type abstraction - Terraform is first implementation; helmfile/packer plug into same contract - Polymorphic ci.data field discriminated by component_type Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/prd/pro-summary-upload.md (2)
206-207:⚠️ Potential issue | 🟠 MajorDefine redaction rules for
ci.data.outputs(not justoutput_log).The PRD secures
output_log, butci.data.outputsis still documented as raw values. Terraform outputs can carry secrets, so this needs an explicit mask/omit rule tied to Terraform sensitive metadata before upload.Suggested PRD patch
- | `ci.data.outputs` | `TerraformOutputData.Outputs` | Terraform output values (raw) | + | `ci.data.outputs` | `TerraformOutputData.Outputs` | Terraform output values with sensitive values masked or omitted |- **Secret masking before upload**: The output log passes through `io.Mask()` (Gitleaks-based, 120+ patterns) before base64 encoding. This ensures secrets never leave the CLI unredacted, matching the security guarantees of all other Atmos output streams. + - **Sensitive Terraform outputs**: Values marked sensitive in Terraform output metadata MUST NOT be uploaded in cleartext. Replace with `"[REDACTED]"` or omit the key.Also applies to: 301-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/prd/pro-summary-upload.md` around lines 206 - 207, Add explicit redaction rules for ci.data.outputs similar to output_log: detect TerraformOutputData.Outputs entries flagged as sensitive (Terraform "sensitive" metadata) and either mask or omit those values before any upload; update the PRD text to state that ci.data.outputs will be filtered according to Terraform sensitive metadata and show the same masking/omission policy as output_log (also apply the same change to the duplicate spots referenced around lines 301-302).
71-73:⚠️ Potential issue | 🟡 MinorAlign “apply-only outputs” requirement with the implementation snippet.
The prose says outputs are apply-only, but the snippet sets
Outputsunconditionally. Add an explicit command/result guard in the PRD snippet to avoid divergent implementations.Suggested PRD patch
- data.Data = &TerraformStatusData{ - ResourceCounts: &tfData.ResourceCounts, - Outputs: extractOutputValues(tfData.Outputs), - } + tfStatus := &TerraformStatusData{ + ResourceCounts: &tfData.ResourceCounts, + } + if isApplyCommand(result) { + tfStatus.Outputs = extractOutputValues(tfData.Outputs) + } + data.Data = tfStatusAlso applies to: 86-89, 202-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/prd/pro-summary-upload.md` around lines 71 - 73, The PRD snippet unconditionally includes the Outputs field (Outputs map[string]any) even though the prose requires “apply-only outputs”; update the example so Outputs is only populated when the operation/result indicates an apply (e.g., add an explicit guard checking command == "apply" or result.IsApplySuccess before setting Outputs). Locate the struct/variable with the Outputs symbol and change the example flow to conditionally assign or include Outputs only in the apply-success branch (and show an empty/omitted Outputs for non-apply commands).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 206-207: Add explicit redaction rules for ci.data.outputs similar
to output_log: detect TerraformOutputData.Outputs entries flagged as sensitive
(Terraform "sensitive" metadata) and either mask or omit those values before any
upload; update the PRD text to state that ci.data.outputs will be filtered
according to Terraform sensitive metadata and show the same masking/omission
policy as output_log (also apply the same change to the duplicate spots
referenced around lines 301-302).
- Around line 71-73: The PRD snippet unconditionally includes the Outputs field
(Outputs map[string]any) even though the prose requires “apply-only outputs”;
update the example so Outputs is only populated when the operation/result
indicates an apply (e.g., add an explicit guard checking command == "apply" or
result.IsApplySuccess before setting Outputs). Locate the struct/variable with
the Outputs symbol and change the example flow to conditionally assign or
include Outputs only in the apply-success branch (and show an empty/omitted
Outputs for non-apply commands).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67057692-024d-48dd-b052-eb18788bf3f3
📒 Files selected for processing (1)
docs/prd/pro-summary-upload.md
- Fix io.Mask() to use WithStdoutCapture (already-masked capture) - Clarify two capture paths: raw for parsing, masked for output_log - Replace sensitive terraform outputs with <MASKED> - Add server-defined output log size limits with truncation - Note deploy→apply conversion for Atmos Pro - Change CI field from typed struct to map[string]any for flexibility - Add cross-reference to instance-status-raw-upload.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/prd/pro-summary-upload.md (1)
177-203: Fix duplicate section numbering for readability.There are two
### 7.sections (“Integration Point” and “Data to Upload”). Renumber the second one to keep the doc easy to navigate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/prd/pro-summary-upload.md` around lines 177 - 203, The document has duplicate heading numbers: both "Integration Point" and "Data to Upload" use "### 7."; rename the second heading (the one titled "Data to Upload") to "### 8." (or the next sequential number) so headings read "### 7. Integration Point" and "### 8. Data to Upload" to restore correct numbering and improve navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 216-217: Update the docs to remove the phrase "Full command
stdout" for `ci.output_log` and clarify that `ci.output_log` contains the
captured stdout which may be truncated (see `ci.truncated` and Section 6), e.g.,
describe it as "Captured command stdout (base64-encoded); may be partial if
truncated" and mention that truncation is controlled/masked by
`WithStdoutCapture`; ensure both `ci.output_log` and `ci.truncated` entries
consistently state the partial/truncation behavior.
- Around line 131-132: The doc incorrectly shows resolving the CI plugin with
ci.GetPlugin(info.Command); change this to use the component type string (e.g.,
info.ComponentType) when calling ci.GetPlugin so the plugin lookup succeeds for
components like "terraform" or "ansible"; then, if the resolved plugin
implements StatusDataProvider, call its BuildStatusData(result, output) to
obtain the map[string]any as described (reference symbols: ci.GetPlugin,
info.ComponentType, StatusDataProvider, BuildStatusData, result, output).
---
Nitpick comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 177-203: The document has duplicate heading numbers: both
"Integration Point" and "Data to Upload" use "### 7."; rename the second heading
(the one titled "Data to Upload") to "### 8." (or the next sequential number) so
headings read "### 7. Integration Point" and "### 8. Data to Upload" to restore
correct numbering and improve navigation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1bb9c395-580f-4a14-a711-9e01d69a0e87
📒 Files selected for processing (1)
docs/prd/pro-summary-upload.md
Comment on lines
+131
to
+132
| 2. Resolve the CI plugin for the current component type via `ci.GetPlugin(info.Command)`. | ||
| 3. If the plugin implements `StatusDataProvider`, call `BuildStatusData(result, output)` to get `map[string]any`. |
Contributor
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
#!/bin/bash
# Verify what key GetPlugin expects and how it's used in code.
rg -n -C3 --type go '\bfunc\s+GetPlugin\s*\('
rg -n -C3 --type go '\bGetPlugin\s*\('
rg -n -C3 --type go '\btype\s+Plugin\b|\bGetType\s*\('Repository: cloudposse/atmos
Repository: cloudposse/atmos
Exit code: 0
stdout:
pkg/ci/plugin_registry.go-45-}
pkg/ci/plugin_registry.go-46-
pkg/ci/plugin_registry.go-47-// GetPlugin returns a CI plugin by component type.
pkg/ci/plugin_registry.go:48:func GetPlugin(componentType string) (plugin.Plugin, bool) {
pkg/ci/plugin_registry.go-49- defer perf.Track(nil, "plugin.GetPlugin")()
pkg/ci/plugin_registry.go-50-
pkg/ci/plugin_registry.go-51- pluginsMu.RLock()
pkg/ci/plugin_registry_test.go-18- require.NoError(t, err)
pkg/ci/plugin_registry_test.go-19-
pkg/ci/plugin_registry_test.go-20- // Verify registration.
pkg/ci/plugin_registry_test.go:21: p, ok := GetPlugin("test-type")
pkg/ci/plugin_registry_test.go-22- assert.True(t, ok)
pkg/ci/plugin_registry_test.go-23- assert.Equal(t, "test-type", p.GetType())
pkg/ci/plugin_registry_test.go-24- })
--
pkg/ci/plugin_registry.go-45-}
pkg/ci/plugin_registry.go-46-
pkg/ci/plugin_registry.go-47-// GetPlugin returns a CI plugin by component type.
pkg/ci/plugin_registry.go:48:func GetPlugin(componentType string) (plugin.Plugin, bool) {
pkg/ci/plugin_registry.go-49- defer perf.Track(nil, "plugin.GetPlugin")()
pkg/ci/plugin_registry.go-50-
pkg/ci/plugin_registry.go-51- pluginsMu.RLock()
--
pkg/ci/executor.go-115- return nil, nil
pkg/ci/executor.go-116- }
pkg/ci/executor.go-117-
pkg/ci/executor.go:118: pl, ok := GetPlugin(componentType)
pkg/ci/executor.go-119- if !ok {
pkg/ci/executor.go-120- log.Debug("No CI plugin registered for component type", "component_type", componentType)
pkg/ci/executor.go-121- return nil, nil
pkg/ci/mock_plugin_test.go-79-}
pkg/ci/mock_plugin_test.go-80-
pkg/ci/mock_plugin_test.go-81-// GetType mocks base method.
pkg/ci/mock_plugin_test.go:82:func (m *MockPlugin) GetType() string {
pkg/ci/mock_plugin_test.go-83- m.ctrl.T.Helper()
pkg/ci/mock_plugin_test.go-84- ret := m.ctrl.Call(m, "GetType")
pkg/ci/mock_plugin_test.go-85- ret0, _ := ret[0].(string)
--
pkg/ci/mock_plugin_test.go-87-}
pkg/ci/mock_plugin_test.go-88-
pkg/ci/mock_plugin_test.go-89-// GetType indicates an expected call of GetType.
pkg/ci/mock_plugin_test.go:90:func (mr *MockPluginMockRecorder) GetType() *MockPluginGetTypeCall {
pkg/ci/mock_plugin_test.go-91- mr.mock.ctrl.T.Helper()
pkg/ci/mock_plugin_test.go-92- call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetType", reflect.TypeOf((*MockPlugin)(nil).GetType))
pkg/ci/mock_plugin_test.go-93- return &MockPluginGetTypeCall{Call: call}
--
pkg/component/registry_test.go-20- commands []string
pkg/component/registry_test.go-21-}
pkg/component/registry_test.go-22-
pkg/component/registry_test.go:23:func (t *testProvider) GetType() string {
pkg/component/registry_test.go-24- return t.componentType
pkg/component/registry_test.go-25-}
pkg/component/registry_test.go-26-
--
pkg/component/registry_test.go-68-
pkg/component/registry_test.go-69- retrieved, ok := GetProvider("test")
pkg/component/registry_test.go-70- assert.True(t, ok)
pkg/component/registry_test.go:71: assert.Equal(t, "test", retrieved.GetType())
pkg/component/registry_test.go-72-}
pkg/component/registry_test.go-73-
pkg/component/registry_test.go-74-func TestRegisterErrors(t *testing.T) {
--
pkg/component/registry_test.go-129- retrieved, ok := GetProvider("test")
pkg/component/registry_test.go-130- assert.True(t, ok)
pkg/component/registry_test.go-131- assert.NotNil(t, retrieved)
pkg/component/registry_test.go:132: assert.Equal(t, "test", retrieved.GetType())
pkg/component/registry_test.go-133-
pkg/component/registry_test.go-134- // Not found case.
pkg/component/registry_test.go-135- _, ok = GetProvider("nonexistent")
--
pkg/component/registry_test.go-235- // Found case - should not panic.
pkg/component/registry_test.go-236- retrieved := MustGetProvider("test")
pkg/component/registry_test.go-237- assert.NotNil(t, retrieved)
pkg/component/registry_test.go:238: assert.Equal(t, "test", retrieved.GetType())
pkg/component/registry_test.go-239-}
pkg/component/registry_test.go-240-
pkg/component/registry_test.go-241-func TestMustGetProviderPanic(t *testing.T) {
--
pkg/component/provider.go-13-// extensible component type support without modifying core code.
pkg/component/provider.go-14-type ComponentProvider interface {
pkg/component/provider.go-15- // GetType returns the component type identifier (e.g., "terraform", "helmfile", "mock").
pkg/component/provider.go:16: GetType() string
pkg/component/provider.go-17-
pkg/component/provider.go-18- // GetGroup returns the component group for categorization.
pkg/component/provider.go-19- // Examples: "Infrastructure as Code", "Kubernetes", "Images", "Testing"
--
pkg/component/registry.go-34- return fmt.Errorf("component provider is nil: %w", errUtils.ErrComponentProviderNil)
pkg/component/registry.go-35- }
pkg/component/registry.go-36-
pkg/component/registry.go:37: componentType := provider.GetType()
pkg/component/registry.go-38- if componentType == "" {
pkg/component/registry.go-39- return fmt.Errorf("component type empty: %w", errUtils.ErrComponentTypeEmpty)
pkg/component/registry.go-40- }
--
pkg/component/registry.go-114- infos := make([]ComponentInfo, 0, len(registry.providers))
pkg/component/registry.go-115- for _, provider := range registry.providers {
pkg/component/registry.go-116- infos = append(infos, ComponentInfo{
pkg/component/registry.go:117: Type: provider.GetType(),
pkg/component/registry.go-118- Group: provider.GetGroup(),
pkg/component/registry.go-119- Commands: provider.GetAvailableCommands(),
pkg/component/registry.go-120- })
--
pkg/component/mock/mock_test.go-13-
pkg/component/mock/mock_test.go-14-func TestMockComponentProvider_GetType(t *testing.T) {
pkg/component/mock/mock_test.go-15- provider := &MockComponentProvider{}
pkg/component/mock/mock_test.go:16: assert.Equal(t, "mock", provider.GetType())
pkg/component/mock/mock_test.go-17-}
pkg/component/mock/mock_test.go-18-
pkg/component/mock/mock_test.go-19-func TestMockComponentProvider_GetGroup(t *testing.T) {
--
pkg/component/ansible/ansible_test.go-13-
pkg/component/ansible/ansible_test.go-14-func TestAnsibleComponentProvider_GetType(t *testing.T) {
pkg/component/ansible/ansible_test.go-15- provider := &AnsibleComponentProvider{}
pkg/component/ansible/ansible_test.go:16: assert.Equal(t, "ansible", provider.GetType())
pkg/component/ansible/ansible_test.go-17-}
pkg/component/ansible/ansible_test.go-18-
pkg/component/ansible/ansible_test.go-19-func TestAnsibleComponentProvider_GetGroup(t *testing.T) {
--
pkg/component/ansible/ansible.go-26-}
pkg/component/ansible/ansible.go-27-
pkg/component/ansible/ansible.go-28-// GetType returns the component type identifier.
pkg/component/ansible/ansible.go:29:func (a *AnsibleComponentProvider) GetType() string {
pkg/component/ansible/ansible.go-30- defer perf.Track(nil, "ansible.GetType")()
pkg/component/ansible/ansible.go-31-
pkg/component/ansible/ansible.go-32- return "ansible"
--
pkg/component/mock/mock.go-29-}
pkg/component/mock/mock.go-30-
pkg/component/mock/mock.go-31-// GetType returns the component type identifier.
pkg/component/mock/mock.go:32:func (m *MockComponentProvider) GetType() string {
pkg/component/mock/mock.go-33- defer perf.Track(nil, "mock.GetType")()
pkg/component/mock/mock.go-34-
pkg/component/mock/mock.go-35- return "mock"
--
pkg/ci/plugins/terraform/plugin.go-21-var defaultTemplates embed.FS
pkg/ci/plugins/terraform/plugin.go-22-
pkg/ci/plugins/terraform/plugin.go-23-// Plugin implements plugin.Plugin for Terraform.
pkg/ci/plugins/terraform/plugin.go:24:type Plugin struct{}
pkg/ci/plugins/terraform/plugin.go-25-
pkg/ci/plugins/terraform/plugin.go-26-// Ensure Plugin implements plugin.Plugin.
pkg/ci/plugins/terraform/plugin.go-27-var _ plugin.Plugin = (*Plugin)(nil)
--
pkg/ci/plugins/terraform/plugin.go-35-}
pkg/ci/plugins/terraform/plugin.go-36-
pkg/ci/plugins/terraform/plugin.go-37-// GetType returns the component type.
pkg/ci/plugins/terraform/plugin.go:38:func (p *Plugin) GetType() string {
pkg/ci/plugins/terraform/plugin.go-39- return "terraform"
pkg/ci/plugins/terraform/plugin.go-40-}
pkg/ci/plugins/terraform/plugin.go-41-
--
pkg/ci/plugins/terraform/plugin_test.go-14-
pkg/ci/plugins/terraform/plugin_test.go-15-func TestPlugin_GetType(t *testing.T) {
pkg/ci/plugins/terraform/plugin_test.go-16- p := &Plugin{}
pkg/ci/plugins/terraform/plugin_test.go:17: assert.Equal(t, "terraform", p.GetType())
pkg/ci/plugins/terraform/plugin_test.go-18-}
pkg/ci/plugins/terraform/plugin_test.go-19-
pkg/ci/plugins/terraform/plugin_test.go-20-func TestPlugin_GetHookBindings(t *testing.T) {
--
pkg/ci/plugin_registry_test.go-20- // Verify registration.
pkg/ci/plugin_registry_test.go-21- p, ok := GetPlugin("test-type")
pkg/ci/plugin_registry_test.go-22- assert.True(t, ok)
pkg/ci/plugin_registry_test.go:23: assert.Equal(t, "test-type", p.GetType())
pkg/ci/plugin_registry_test.go-24- })
pkg/ci/plugin_registry_test.go-25-
pkg/ci/plugin_registry_test.go-26- t.Run("nil provider", func(t *testing.T) {
--
pkg/ci/plugin_registry_test.go-61- t.Run("event found", func(t *testing.T) {
pkg/ci/plugin_registry_test.go-62- p := GetPluginForEvent("after.test-terraform.plan")
pkg/ci/plugin_registry_test.go-63- require.NotNil(t, p)
pkg/ci/plugin_registry_test.go:64: assert.Equal(t, "test-terraform", p.GetType())
pkg/ci/plugin_registry_test.go-65- })
pkg/ci/plugin_registry_test.go-66-
pkg/ci/plugin_registry_test.go-67- t.Run("event not found", func(t *testing.T) {
--
pkg/ci/plugin_registry.go-23- return errUtils.ErrNilParam
pkg/ci/plugin_registry.go-24- }
pkg/ci/plugin_registry.go-25-
pkg/ci/plugin_registry.go:26: componentType := p.GetType()
pkg/ci/plugin_registry.go-27- if componentType == "" {
pkg/ci/plugin_registry.go-28- return errUtils.Build(errUtils.ErrInvalidConfig).
pkg/ci/plugin_registry.go-29- WithExplanation("Plugin has empty type").
--
pkg/ci/internal/plugin/types.go-65-// Plugins own all action logic via Handler callbacks in their HookBindings.
pkg/ci/internal/plugin/types.go-66-// Unlike Provider (which represents CI platforms like GitHub/GitLab), this interface
pkg/ci/internal/plugin/types.go-67-// represents component types (terraform, helmfile) and their CI behavior.
pkg/ci/internal/plugin/types.go:68:type Plugin interface {
pkg/ci/internal/plugin/types.go-69- // GetType returns the component type (e.g., "terraform", "helmfile").
pkg/ci/internal/plugin/types.go:70: GetType() string
pkg/ci/internal/plugin/types.go-71-
pkg/ci/internal/plugin/types.go-72- // GetHookBindings returns all hook bindings for this plugin.
pkg/ci/internal/plugin/types.go-73- // Each binding declares an event and a Handler callback that owns all action logic.
--
pkg/ci/executor_test.go-54- bindings []plugin.HookBinding
pkg/ci/executor_test.go-55-}
pkg/ci/executor_test.go-56-
pkg/ci/executor_test.go:57:func (s *stubPlugin) GetType() string {
pkg/ci/executor_test.go-58- return s.componentType
pkg/ci/executor_test.go-59-}
pkg/ci/executor_test.go-60-
Use component type for plugin resolution, not command.
Line 131 proposes ci.GetPlugin(info.Command), but GetPlugin expects a component type string (like "terraform" or "ansible"), not a subcommand like "plan" or "apply". Passing info.Command would fail plugin lookup.
Proposed doc fix
-2. Resolve the CI plugin for the current component type via `ci.GetPlugin(info.Command)`.
+2. Resolve the CI plugin for the current component type via `ci.GetPlugin(<componentType>)`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/prd/pro-summary-upload.md` around lines 131 - 132, The doc incorrectly
shows resolving the CI plugin with ci.GetPlugin(info.Command); change this to
use the component type string (e.g., info.ComponentType) when calling
ci.GetPlugin so the plugin lookup succeeds for components like "terraform" or
"ansible"; then, if the resolved plugin implements StatusDataProvider, call its
BuildStatusData(result, output) to obtain the map[string]any as described
(reference symbols: ci.GetPlugin, info.ComponentType, StatusDataProvider,
BuildStatusData, result, output).
Comment on lines
+216
to
+217
| | `ci.output_log` | base64(captured stdout) | Full command stdout, masked via `WithStdoutCapture`, base64-encoded | | ||
| | `ci.truncated` | size check | Whether the output log was truncated due to size limits | |
Contributor
There was a problem hiding this comment.
Clarify that ci.output_log may be partial.
Line 216 says “Full command stdout,” but line 217 and Section 6 explicitly allow truncation. Tighten wording to prevent API-contract ambiguity.
Proposed doc fix
-| `ci.output_log` | base64(captured stdout) | Full command stdout, masked via `WithStdoutCapture`, base64-encoded |
+| `ci.output_log` | base64(captured stdout) | Command stdout (masked via `WithStdoutCapture`, base64-encoded; may be truncated) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/prd/pro-summary-upload.md` around lines 216 - 217, Update the docs to
remove the phrase "Full command stdout" for `ci.output_log` and clarify that
`ci.output_log` contains the captured stdout which may be truncated (see
`ci.truncated` and Section 6), e.g., describe it as "Captured command stdout
(base64-encoded); may be partial if truncated" and mention that truncation is
controlled/masked by `WithStdoutCapture`; ensure both `ci.output_log` and
`ci.truncated` entries consistently state the partial/truncation behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/prd/pro-summary-upload.md`:
- Around line 153-203: The document has duplicate section numbers for the
headings "Integration Point" and "Data to Upload"; update the heading numbers so
they are sequential by renaming "7. Integration Point" to "8. Integration Point"
and renaming the subsequent "7. Data to Upload" to "9. Data to Upload" (ensure
any in-text references or the table of contents that mention these section
numbers are updated accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c13d652-ef19-4ce7-a8e7-5474a09835a0
📒 Files selected for processing (1)
docs/prd/pro-summary-upload.md
Comment on lines
+153
to
+203
| ### 6. Output Log Size Limits | ||
|
|
||
| The output log can be large (verbose providers, many resources). To prevent oversized payloads, the CLI truncates the output log before base64 encoding. | ||
|
|
||
| **Max size is server-defined:** The CLI fetches the max payload size from the Atmos Pro API at upload time (or caches it). The server returns the limit via a settings/configuration endpoint. This lets the server control the limit without requiring CLI updates. | ||
|
|
||
| **Truncation behavior:** | ||
| - If the masked output exceeds the server-defined max size, truncate from the **beginning** (keep the tail — the most useful part: plan summary, apply result, errors). | ||
| - Add a `"truncated": true` key to the CI map so the server knows the log is incomplete. | ||
| - If the server is unreachable for settings, fall back to a built-in default (e.g., 3MB pre-encoding, which becomes ~4MB after base64). | ||
|
|
||
| **Server settings endpoint:** | ||
| ``` | ||
| GET /api/v1/settings | ||
| ``` | ||
| Response (relevant fields): | ||
| ```json | ||
| { | ||
| "max_output_log_bytes": 3145728 | ||
| } | ||
| ``` | ||
|
|
||
| The CLI caches this value for the duration of the command execution. If the endpoint is unavailable, the built-in default is used. | ||
|
|
||
| ### 7. Integration Point | ||
|
|
||
| The upload needs two views of the command output: | ||
|
|
||
| 1. **Raw output** — for parsing (resource counts, warnings, errors, terraform outputs). The parser's regex patterns expect unmodified terraform output; masked strings (`<MASKED>`) could break extraction. | ||
| 2. **Masked output** — for the `output_log` field. Secrets must be redacted before upload. | ||
|
|
||
| `ExecuteShellCommand` supports both via its option system: | ||
| - `WithStdoutCapture(w io.Writer)` captures output **after** `MaskWriter` (masked). | ||
| - A second capture buffer can be added **before** `MaskWriter` to get raw output for parsing. | ||
|
|
||
| Alternatively, if the CI hooks already capture raw output into `HookContext.Output`, reuse that for parsing and only use `WithStdoutCapture` for the masked log. | ||
|
|
||
| ```go | ||
| var maskedOutput bytes.Buffer | ||
| err := ExecuteShellCommand(..., WithStdoutCapture(&maskedOutput)) | ||
|
|
||
| // For parsing: use raw output from CI hook context or pre-mask capture | ||
| result := terraform.ParseOutput(rawOutput, info.SubCommand) | ||
|
|
||
| // For upload: use masked capture | ||
| data.OutputLog = base64.StdEncoding.EncodeToString(maskedOutput.Bytes()) | ||
| ``` | ||
|
|
||
| Pass both the parsed result and the masked buffer to `uploadCommandStatus()`. | ||
|
|
||
| ### 7. Data to Upload |
Contributor
There was a problem hiding this comment.
Fix duplicate section numbering.
Section 6 ends at line 175, then two sections are both labeled "7" (lines 177 and 203). Renumber "7. Integration Point" to section 8 and "7. Data to Upload" to section 9.
📝 Suggested fix
-### 7. Integration Point
+### 8. Integration Point
The upload needs two views of the command output:
...
-### 7. Data to Upload
+### 9. Data to Upload
All fields live under the `ci` block in the payload.🧰 Tools
🪛 LanguageTool
[typographical] ~161-~161: Consider using a typographic close quote here.
Context: ...summary, apply result, errors). - Add a "truncated": true key to the CI map so t...
(EN_QUOTES)
[typographical] ~162-~162: Insert a space between the numerical value and the unit symbol.
Context: ... fall back to a built-in default (e.g., 3MB pre-encoding, which becomes ~4MB after ...
(UNIT_SPACE)
[typographical] ~162-~162: Insert a space between the numerical value and the unit symbol.
Context: ...(e.g., 3MB pre-encoding, which becomes ~4MB after base64). **Server settings endpo...
(UNIT_SPACE)
🪛 markdownlint-cli2 (0.22.0)
[warning] 165-165: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/prd/pro-summary-upload.md` around lines 153 - 203, The document has
duplicate section numbers for the headings "Integration Point" and "Data to
Upload"; update the heading numbers so they are sequential by renaming "7.
Integration Point" to "8. Integration Point" and renaming the subsequent "7.
Data to Upload" to "9. Data to Upload" (ensure any in-text references or the
table of contents that mention these section numbers are updated accordingly).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
InstanceStatusUploadRequestDTO with a nestedCIblock containing resource counts, terraform outputs, warnings, errors, and base64-encoded output log--upload-statusflag +ci.enabled— backward compatible viaomitemptypointerTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit